feat(io/s3): support connect timeout property#950
Conversation
Signed-off-by: A Z <zalabany3@gmail.com>
laskoviymishka
left a comment
There was a problem hiding this comment.
Thanks for picking this up:Dialer.Timeout is the right knob, and threading a single BuildableClient through the proxy and timeout branches is the right shape. Before merge I'd like to see one blocking item and two smaller items addressed:
Value format diverges from PyIceberg: s3.connect-timeout is a documented PyIceberg property whose value is seconds as a float (e.g. "60" or "60.0"; see pyiceberg/io/pyarrow.py and pyiceberg/io/fsspec.py, both of which call float(...), plus the 60.0 example in PyIceberg's configuration.md). time.ParseDuration rejects those values, so any property bag flowing from a Python writer to a Go reader — or any user copying values from PyIceberg docs — will hard-error here. Please accept the seconds-as-number form, with time.ParseDuration as a fallback for Go-flavored strings:
if s, err := strconv.ParseFloat(connectTimeout, 64); err == nil {
timeout = time.Duration(s * float64(time.Second))
} else if d, derr := time.ParseDuration(connectTimeout); derr == nil {
timeout = d
} else {
return nil, fmt.Errorf("invalid s3 connect timeout %q (expected seconds, e.g. \"60\", or duration like \"5s\"): %w", connectTimeout, err)
}It would be good to document the accepted forms next to the S3ConnectTimeout constant, and to add a test for "60" / "60.0".
Reject zero and negative durations. time.ParseDuration("0") succeeds and produces 0, which net.Dialer treats as "no timeout" — the opposite of what a user setting s3.connect-timeout=0 would expect. Negative values parse and make every dial fail with i/o timeout from far away. A if timeout <= 0 { return nil, fmt.Errorf(...) } check after parsing covers both.
Add a proxy + connect-timeout composition test. The structural reason to hoist httpClient into a shared variable is to compose proxy and timeout, but no test sets both. Please add a case that sets both S3ProxyURI and S3ConnectTimeout and asserts the dialer's Timeout and that the transport's Proxy is non-nil — this is the regression that would slip through silently in a future refactor.
Smaller things, non-blocking:
- Removing the
unsupportedS3Propsguard is an observable behavior change forParseAWSConfig(it used to returnunsupported S3 property "..."for this key). Worth a release-notes line. - The proxy branch above uses
'%s'and drops theurl.Parseerror; the new branch nicely uses%q+%w. Aligning the proxy branch in a follow-up would be a small but worthwhile cleanup — out of scope here.
Happy to re-review once the format/zero-value handling and composition test are in.
Fixes #941.
Summary
s3.connect-timeoutfrom the unsupported S3 property path.s3.connect-timeoutas a Go duration and apply it to the AWS SDK buildable client's dialer timeout.Testing
go test ./io/gocloudgit diff --check && go test ./...